Skip to content

Conversation

@NBickford-NV
Copy link
Contributor

After calling stbi_load_gif, apps should free the delays pointer it passed in if delays is non-null. This means that after freeing delays in the stbi__load_gif_main_outofmem error path, stb_image_load should also set delays to NULL. Otherwise if we allocate delays once but we get to stbi__load_gif_main_outofmem on a later frame, then the app sees a non-null delays pointer and tries to free it again.

With this change, running clang++ poc.c -fsanitize=address && ./a.out 490442704-000e388f-3edd-409b-9253-83046ac80317.gif from #1838 now correctly reports "Failed to load GIF" instead of segfaulting.

Forks with JarLob's double-free fix in #1549 were also affected; it implemented this logic on some paths, but not all, and #1838 happens to reach one of the other paths. Moving the delays = NULL logic to stbi__load_gif_main_outofmem on those forks allows us to save a few lines of code.

In addition, since realloc first frees the input pointer (C specification 7.22.3.5 item 1), we shouldn't pass the input to stbi__load_gif_main_outofmem after a realloc -- then we'd get a different double-free. If we remove the temporary pointer and do p = realloc(p, ...) then I think we get the right behavior and save a few lines of code.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant